Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages when necessary optimization library is not loaded #69

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

salbalkus
Copy link
Contributor

Addresses #68 , which arises when the user specifies an optimization library using the optlib parameter that is supported, but they have not currently loaded the library that they have specified.

This PR adds a function that checks if the package specified by optlib has been loaded by the user. It also adds a function to throw an error informing the user that package corresponding to the optlib has not been loaded and to either run using [PackageName] or otherwise specify the correct library. The densratio and densratiofunc functions have been updated to throw the corresponding error in these cases, and unit tests for these cases have been included in runtests.jl.

Additionally, other existing error messages related to the specification of optimization libraries have been made slightly more descriptive and verbose.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.09%. Comparing base (4ffd7a4) to head (b6ae36f).

Files Patch % Lines
src/api/estimators.jl 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #69       +/-   ##
===========================================
+ Coverage   69.65%   83.09%   +13.44%     
===========================================
  Files          18       18               
  Lines         201      207        +6     
===========================================
+ Hits          140      172       +32     
+ Misses         61       35       -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juliohm
Copy link
Member

juliohm commented Jul 8, 2024

@salbalkus appreciate your efforts to address various error messages at once with additional code and logic, but as discussed in the issue, we just need a simple patch release with better error messages (strings inside @error calls), which are already thrown. The code added in this PR does a lot more, and relies on a design that is not good for maintenance.

Can you please simplify the PR to only improve the error messages?

@salbalkus
Copy link
Contributor Author

@juliohm The reason for my contribution is to fix the problem I described in #68 , which will not be addressed by simply changing the @error calls. It seems I have not clearly communicated the issue (partially because I had not totally dug into the entire codebase yet) so let me elaborate.

The @error message calls you referenced are only called if the user attempts to use a DensityRatioEstimator with an optimization library that it hasn't been implemented in. For example, this would occur if the user attempts to call densratio(x, y, KLIEP(); optlib = JuMPLib) because although JuMPLib is a valid option for some estimators, it is not valid for KLIEP.

However, it does not occur if the user calls the DensityRatioEstimator with an optimization library that is implemented but is not yet loaded. For example, if the user calls densratio(x, y, KLIEP(); optlib = Optim) without having first called using Optim, the @error you mentioned will not be reached, because Optim is implemented and Julia will only dispatch on the function with the @error as a fallback when a _densratio function is not already defined for a more specific type of optimization library. As a result of this, the _densratio function will be dispatched on the "correct" version, but an error will be thrown because it is not loaded.

The problem is, as mentioned in #68 , the error that is thrown is MethodError: no method matching iterate(::Nothing) (with no more specific warning from @error) which does not tell the user what they really need to know: that they haven't loaded the correct default library. This is a problem for methods like KLIEP and LSIF where the default libraries are not base Julia. As they often do, if users simply copy and paste any code from the docs other than the very first line, i.e. r = densratio(x_nu, x_de, KLIEP()) or r = densratiofunc(x_nu, x_de, KLIEP()) they will see "no method matching iterate(::Nothing)" which makes it seem like the package doesn't work even in the cases when it is meant to and might drive users away.

I'm definitely open to implementing your feedback and changes on this PR regarding how to design functionality that actually fixes the problem above in a more maintainable way. One improvement, rather than casting the library name into a string and subsetting it (which might break in future releases if the names change), might be to define a dictionary as a global object that maps the object names into package names, i.e. OptimLib => Optim. If you have more specific constraints on the design of the package that you'd like to enforce, please let me know and I'd be happy to work within those bounds!

@juliohm
Copy link
Member

juliohm commented Jul 8, 2024

This is the error message I get from the MWE in #69 :

image

It is calling the @error correctly, just the message that needs to be improved. Can you please share the error message you are getting? Why is it not hitting the same code?

@juliohm
Copy link
Member

juliohm commented Jul 8, 2024

For example, if the user calls densratio(x, y, KLIEP(); optlib = Optim) without having first called using Optim, the @error you mentioned will not be reached, because Optim is implemented and Julia will only dispatch on the function with the @error as a fallback when a _densratio function is not already defined for a more specific type of optimization library. As a result of this, the _densratio function will be dispatched on the "correct" version, but an error will be thrown because it is not loaded.

That seems like a separate issue, which we should address in a separate PR. The fix could be as simple as improving the error message too. Just mentioning that the error may occur when the library is not loaded. No need for additional logic I think, but please let me know if I am missing something.

@salbalkus
Copy link
Contributor Author

This is the error message I get from the MWE in #69 :

image

It is calling the @error correctly, just the message that needs to be improved. Can you please share the error message you are getting? Why is it not hitting the same code?

My apologies, @juliohm. It think the MWE from #68 was not showing the "not implemented" error due to an issue with my VSCode setup not properly displaying logs, which I am unable to reproduce; when I run it in the terminal I get the same result as you. That being said, a similar situation actually does occur with KMM; the "not implemented" error message doesn't appear if one runs

using DensityRatioEstimation
densratio([0], [0], KMM())

yielding

ERROR: MethodError: no method matching _kmm_ratios(::Matrix{Float64}, ::Matrix{Float64}, ::KMM{Float64}, ::Type{JuMPLib})

Closest candidates are:
  _kmm_ratios(::Any, ::Any, ::uKMM, ::Type{JuliaLib})
   @ DensityRatioEstimation C:\Users\salba\.julia\packages\DensityRatioEstimation\1Mu92\src\kmm\julia.jl:5

since KMM is missing a "default" function for _kmm_ratios to throw the error.

In this case, is there a reason that @error is used to print an error log instead of throwing an exception using error or throw? From the docs the @error macro is intended for "nonfatal errors", though obviously failing to load the correct optimization package should result in a fatal error. Can I open a new pull request and replace @error "msg" with error("msg") with improved messages? This should also make it easier to test using @test_throws.

@juliohm
Copy link
Member

juliohm commented Jul 8, 2024 via email

@juliohm
Copy link
Member

juliohm commented Jul 16, 2024

@salbalkus did you have a chance to take a look into this? We just need to throw and actual error with a more informative error message.

@salbalkus
Copy link
Contributor Author

@salbalkus did you have a chance to take a look into this? We just need to throw and actual error with a more informative error message.

Yes, I've written more informative error messages. Currently I'm setting up unit tests to make sure the code is properly covered and they're being thrown in the right situation. I can open a pull request tonight.

@salbalkus
Copy link
Contributor Author

This new commit reverts my previous change to check if the necessary libraries are loaded and adds more maintainable error throwing.

  • I've replaced the previous code with a few basic functions in utils.jl to construct informative error message strings and throw errors from every default function that had previously been adorned with @error. Let me know if these functions belong in a different .jl file.
  • I've also replaced the unit tests to check that the necessary errors are thrown. Currently they are using the generic ErrorException; happy to define a custom type if you feel it is needed.
  • Also, a few errors were not being reached properly; I made some small changes to the parameter typing in the "default" functions to ensure intended behavior.

@juliohm
Copy link
Member

juliohm commented Jul 17, 2024

That is really good! Thanks for the awesome contribution!

Merging and releasing a patch 👍🏽

@juliohm juliohm merged commit b4bd09c into JuliaML:master Jul 17, 2024
6 checks passed
@salbalkus
Copy link
Contributor Author

Thank you @juliohm - happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants